Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hwp_pcu agent for the hwp phase compensation unit operations #588

Merged
merged 12 commits into from
Dec 18, 2023

Conversation

17-sugiyama
Copy link
Contributor

The hwp_pcu agent controls the phase compensation unit (PCU) for the CHWP rotation system.
The PCU will be installed in the CHWP system of all SATs. The SAT2 has already successfully installed this unit.
I would like to add this hwp_pcu agent to the socs.

PCU increases the motor efficiency of the CHWP motor.
PCU can also stop the CHWP rotation without disabling the motor driving system.
The hardware and software of PCU are described in this slide.

This agent is tested in the lab, and will be tested with TSAT at site.
Hopefully this agent will be included in the hwp supervisor agent.

@17-sugiyama 17-sugiyama self-assigned this Dec 6, 2023
@17-sugiyama 17-sugiyama requested a review from jlashner December 6, 2023 07:27
@ykyohei ykyohei self-requested a review December 8, 2023 17:13

**Task** - Send commands to the phase compensation unit.
off: The compensation phase is zero.
on_1:The compensation phase is +120 deg.
Copy link
Contributor

@ykyohei ykyohei Dec 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we change from 'on_1' to '+120deg', 'on_2' to '-120deg'?
Is it not allowed in python?

At least I would like to change the status message.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be fine as it is because "on_1" does not necessarily mean +120 deg. We intend to include the hwp_pcu in the hwp_supervisor, so more accurate compensated angle can be described in hwp_supervisor.

@ykyohei
Copy link
Contributor

ykyohei commented Dec 8, 2023

I think we also need to edit docs/index.rst, and add docs/agents/hwp_pcu.rst to be consistent with other agents.

Copy link
Collaborator

@jlashner jlashner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this really nice code! A few comments inline about usage of locking, and a couple of questions and requests:

  • Do you think you can write up a docs page that includes the information from your slides about what this does / how to use it? You can check out existing doc pages like this for how these should be structured
  • This agent structure, with multiple threads accessing a single hardware object with lock-outs is not really the best way to design an agent like this, as described in this discussion. I won't request that this agent is restructured if you don't want to since this is already very close to being mergeable, however if you are interested I am very happy to help restructure in such a way!

@ykyohei
Copy link
Contributor

ykyohei commented Dec 18, 2023

I have finished the requested modification of the code.
@17-sugiyama please check if it looks good.
I would like to set this the agent up for satp3 system as soon as possible. We confirmed that the hardware is ready to use, by using un-official python scripts.

The hardware is connected to nuc-hk1-satp3 over usb and the port is /dev/ttyACM0.
I would like to ask admin to fix this port to /dev/HWPPCU .
https://github.com/simonsobs/site-ansible/blob/44cc4107243208bd5b9cb2ec1a72b7ee4b08ec61/hosts.txt#L191C19-L191C47

@ykyohei ykyohei requested a review from jlashner December 18, 2023 15:17
Copy link
Collaborator

@jlashner jlashner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing all of the comments. Looks good to me!

@jlashner jlashner merged commit 99d3c68 into main Dec 18, 2023
7 checks passed
@jlashner jlashner deleted the hwp_pcu-agent branch December 18, 2023 15:37
hnakata-JP pushed a commit that referenced this pull request Apr 12, 2024
* hwp_pcu is added.

* Update agent.py

* Update agent.py

* Update hwp_pcu.py

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update agent.py

* make timeout longer and small fixes

* fix get_status

* fix send_command

* add draft of docs

* update draft of docs

* small fixes of docs

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: ykyohei <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants